Code Review: PR #357 (perf/ops hardening)

Recommendation: APPROVE

Summary

Four independent, behavior-preserving ops/perf changes: HTTP response compression (Brotli/Gzip), a DB-connectivity /health check, a wall-clock timeout on UnitOfWork.ExecuteInTransactionAsync, and a slow-query warning-log interceptor. No contract surface (DTO, endpoint, Zod schema) is touched, so the backward-compat guard and contract-aligner are N/A by gate, not by omission.

Findings

Medium

Unauthenticated /health now performs a live DB round trip with no rate limit or cache

• Location: src/Orbit.Infrastructure/Services/DatabaseHealthCheck.cs:19 (wired via src/Orbit.Api/Extensions/WebApplicationExtensions.cs:64-80; registered in src/Orbit.Api/Extensions/ServiceCollectionExtensions.BackgroundJobs.cs:38)
• Before this PR, GET /health (AllowAnonymous, no [DistributedRateLimit]) only checked an in-memory BackgroundServiceHealthCheck. It now also resolves a scoped OrbitDbContext and calls Database.CanConnectAsync() on every hit — a real round trip through the same request-path Npgsql pool real traffic uses (EfMaxPoolSize = 15, sized tightly against Supabase's connection ceiling per DatabaseConnectionSettings.cs's own doc comment). ASP.NET Core health checks have no built-in caching.
• Risk: an anonymous burst against /health (no auth, no rate limit) competes 1:1 with legitimate request-path connections for a small pool, which could degrade or 503 real traffic during a flood — a DoS lever that did not exist before this change.
• Suggested fix: wrap DatabaseHealthCheck's CanConnectAsync result in a short TTL cache (a few seconds is enough for a liveness probe), or add [DistributedRateLimit] / a lightweight per-IP limiter on /health.

Low / Info (non-blocking)

• The BREACH-safety WHY comment in ServiceCollectionExtensions.Infrastructure.cs:260 ("no attacker-reflected secrets") is imprecise: AuthController responses (verify-code, google) do return tokens alongside reflected/attacker-influenced fields in the same JSON body, which is the shape BREACH targets. The actual reason compression is safe here is that auth is Bearer-header-only (no cookie-based session), so a cross-origin page can't force a victim's browser to auto-replay authenticated requests. The conclusion holds; the stated reasoning would go stale if cookie-based auth is ever added. Worth a reword, not a blocker.
• DatabaseConnectionSettings.TransactionTimeoutSeconds / SlowQueryThresholdMilliseconds have no startup bounds validation, unlike the guarded settings in ValidateOrbitSecuritySettings. Config-only risk, not exploitable.
• application/json was already present in ResponseCompressionDefaults.MimeTypes, so half of the explicit .Concat([...]) addition in AddResponseCompression is a harmless no-op duplicate; application/problem+json is the genuinely new, needed addition.

What's good

• The transaction-timeout/caller-cancel distinction in UnitOfWork.ExecuteInTransactionAsync (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) is correctly reasoned and has dedicated tests for both branches, including the ambient-transaction fast path staying untouched.
• ORBIT0002 compliance preserved: the transaction catch block still only does ChangeTracker.Clear(); throw;, relying on await using scope disposal for rollback — no redundant explicit RollbackAsync().
• SlowQueryCommandInterceptor correctly logs via the [LoggerMessage] source-generator pattern with PascalCase structured properties, at Warning level, and is safe against sensitive-data exposure since EnableSensitiveDataLogging() is confirmed absent repo-wide.
• Response compression correctly excludes text/event-stream (ChatController's SSE endpoint) since that content type was never added to the MIME allowlist — no risk of breaking streaming responses.

Deferred — N/A dimensions

• Dimension 9 (Parity web/mobile), 11 (contract drift), 14 (FEATURES.md parity) — N/A: no DTO, Controller route, Zod schema, or user-facing surface touched anywhere in this diff.
• Cross-repo checks (contract-aligner, packages/shared backward-compat) — not applicable rather than unverifiable: determined from the api-side diff alone that no contract surface changed.
• Build/Unit Tests — not run here; covered by separate required CI checks on this PR.

Test coverage spot-checked: UnitOfWorkTests covers timeout→TimeoutException-and-rolled-back and caller-cancel→OperationCanceledException cases; DatabaseHealthCheckTests covers reachable/unreachable; SlowQueryCommandInterceptorTests covers above/at/below threshold. All ten mechanical test-fixture updates for the new DatabaseConnectionSettings constructor argument were confirmed complete via a repo-wide grep — no stale call sites remain.

Recommendation

Approve as-is; the single Medium finding (unauthenticated /health now doing a live DB round trip with no cache/rate-limit) is a defense-in-depth gap, not a blocker — safe to land with a tracked follow-up to add a short TTL cache or [DistributedRateLimit] on /health before it sees adversarial traffic volume.

Code Review: PR #357 — perf(api): compression, DB health check, txn timeout, slow-query logging

Recommendation: NEEDS WORK

Summary

Three of four changes (compression, DB health check, slow-query logging) are clean and well-tested. The transaction-timeout change (UnitOfWork.ExecuteInTransactionAsync, 120s ceiling) has an unconsidered side effect: it wraps pre-existing external HTTP I/O in RunCalendarAutoSyncCommand (sequential, paginated Google Calendar API fetches) inside the same wall-clock budget. Users with several/large calendars can plausibly exceed 120s, causing a silent, indefinitely-recurring background-sync failure that bypasses the handler's existing graceful MarkCalendarSyncTransientError degradation path.

Findings

High

New transaction wall-clock timeout can silently and permanently break Google Calendar auto-sync for users with several/large calendars

• location: src/Orbit.Infrastructure/Persistence/UnitOfWork.cs:40-42 (new ceiling), consumed by src/Orbit.Application/Calendar/Commands/RunCalendarAutoSyncCommand.cs:102-113
• RunCalendarAutoSyncCommand is not IIdempotentCommand, so this call creates a brand-new transaction + fresh 120s timeout (not an ambient join). FetchAndProcessLocked calls deps.EventFetcher.FetchAsync(...) — an external Google Calendar HTTP fetch — inside that same token, before any DB write. GoogleCalendarEventFetcher.FetchAsync lists calendars then loops sequentially per calendar; GoogleCalendarApi.ListEventsAsync itself paginates with a sequential do...while HTTP loop per calendar (30s timeout per call, HttpClients:DefaultTimeoutSeconds). Nothing bounds the aggregate across calendars/pages.
• When the aggregate exceeds 120s, the resulting OperationCanceledException is explicitly excluded from FetchAndProcessLocked's own Google-API-error catch (ex is not OperationCanceledException), so it propagates past the graceful MarkCalendarSyncTransientError path, becomes TimeoutException at the UnitOfWork boundary, and rolls back. Since GoogleCalendarLastSyncedAt never advances, the same user is retried every 15-minute tick indefinitely (not blocked by the 4-hour dedupe window, which only engages once a sync reaches any terminal state) — a silent, permanent-until-fixed degradation with no user-visible status change.
• DatabaseConnectionSettings.TransactionTimeoutSeconds's own doc comment says AI/batch network I/O is deliberately excluded from this ceiling via a separate AI:BatchNetworkTimeoutSeconds — confirming the author's intent was to keep external I/O out of this boundary, but the Calendar-sync path (pre-existing, unchanged by this diff) violates that intent.
• Fix: move deps.EventFetcher.FetchAsync(...) outside ExecuteInTransactionAsync in FetchAndProcess (it's read-only and doesn't need the transaction/advisory lock); only the reconcile+write phase needs the 120s-bounded transaction. As a safety net, also catch TimeoutException in FetchAndProcessLocked alongside the existing Google-API-error catch so failures still call MarkCalendarSyncTransientError. Add a regression test with a fake slow ICalendarEventFetcher.

Medium

New timeout tests only exercise Task.Delay cancellation, not a real in-flight DB-command cancellation — tests/Orbit.Infrastructure.Tests/Persistence/UnitOfWorkTests.cs:526-568. The feature targets "a transaction that wedges between commands," but neither new test has a real DB command outstanding when the timeout fires. Recommend adding one test where the operation delegate awaits a genuinely slow DB call (e.g. via an interceptor that stalls execution) to confirm the timeout still fires and rolls back cleanly against a real command, not just an in-memory delay.

Subagents

• security-reviewer: dispatched async, did not return within session; compensating manual pass found no Critical/High issue (BREACH reasoning holds — JWT Bearer is non-ambient, CORS uses an explicit origin allowlist; /health leaks no exception detail; SlowQueryCommandInterceptor logs CommandText only, not parameter values, and the only raw-SQL call site nearby is parameterized; compression middleware ordering relative to auth is correct).
• contract-aligner: N/A, no DTO/Controller route/packages/shared change in this diff.

Validation

Build/Tests: N/A in this review session (sandbox could not run dotnet build/dotnet test); PR description states all 4,904 tests pass locally, not independently re-verified here.

Deferred

• DESIGN.md/AI-slop, Parity, i18n, FEATURES.md parity: N/A, backend ops/perf-only diff, no UI/contract/feature-surface change.
• /health's new database check entry is additive to the existing checks array; not cross-checked against orbit-ui-mobile (not available in this session) in case any client parses /health specifically.
• The formal Phase-6 adversarial-skeptic subagent for the High finding above did not return within session; the equivalent adversarial checks (ambient-transaction-join check, retry/circuit-breaker check, pagination-depth check, catch-clause-reachability check) were performed directly against source with file:line evidence in place of the subagent pass.

What's good

Compression, DB health check, and slow-query logging are clean, minimal, correctly ordered in the DI/middleware pipeline, fully comment-policy-compliant (every comment carries a WHY + URL), and ORBIT0002-compliant (no explicit rollback inside using-scoped transactions). The OperationCanceledException vs TimeoutException disambiguation logic is correct by manual trace. All existing UnitOfWork test-constructor call sites were updated in lockstep.

Recommendation

Fix the High finding (keep external Calendar HTTP I/O outside the new transaction/timeout boundary, and catch TimeoutException gracefully) before merge. The Medium test-coverage note can land as a fast follow. Everything else in the PR can ship as-is once the timeout/Calendar interaction is fixed.

🤖 Generated with orbit-api /pr-review